Refactoring for adding other sources #4

savnadya · 2024-02-28T14:28:15Z

No description provided.

l0kix2 · 2024-02-28T14:43:59Z

config.go

@@ -73,6 +76,8 @@ type YtsaurusConfig struct {
 	DebugUsernames []string `yaml:"debug_usernames"`
 	// DebugGroupnames is a list of YTsaurus groupnames for which app will print more debug info in logs.
 	DebugGroupnames []string `yaml:"debug_groupnames"`
+
+	SourceAttributeName *string `yaml:"source_attribute_name"`


As with other fields it is ok to have one empty value: empty string. You making own life harder making it pointer.

Let's make it simple string as we discussed + add != "" check in app start (ytsaurus object creation to be specific).

l0kix2 · 2024-02-28T15:04:38Z

ytsaurus_helpers.go

@@ -40,31 +45,37 @@ func doGetAllYtsaurusUsers(ctx context.Context, client yt.Client) ([]YtsaurusUse
 	var users []YtsaurusUser
 	for _, ytUser := range response {
 		var bannedSince time.Time
-		if ytUser.BannedSince != "" {
-			bannedSince, err = time.Parse(appTimeFormat, ytUser.BannedSince)
+		if bannedSinceRaw, ok := ytUser.Attrs[bannedSinceAttributeName]; ok && bannedSinceRaw != "" {


Couldn't Attrs be nil? I've had such panics when I've run tests here https://github.com/nebius/ytsaurus-active-directory-integration/pull/3/files before I've add a check for nil.

l0kix2 · 2024-02-28T16:18:48Z

source_models.go

@@ -0,0 +1,119 @@
+package main


Code in this file should be moved to the other files (packages in the future) to be flexible and aligned with golang philosophy.
In short, golang interfaces are not intended to be used like interfaces in OOP languages, they are more like duck-typing in dynamic languages. There couple rules of thumb here:

interfaces are declared in the place of usage (client code), often such interfaces are private for client package;

functions should accept interfaces and return structs (it is very good rule for New functions espessially).

This is an article i've googled which can give the idea: https://alok87.in/blog/interfaces-go

Therefore:

All interfaces should be defined in the place of usage:

Source should move in app.go

SourceUser, SourceGroup in diff.go

SourceGroupWithMembers also in diff.go

AzureUser, AzureGroup and everything Azure should be in azure*.go

NewSource{User,Group} functions shouldn't exists. It should be NewAzureUser (and NewLdapUser in the future) in azure*.go which should return AzureUser struct (not SourceUser interface).

l0kix2 · 2024-02-28T16:20:11Z

app_test.go

@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"fmt"
+	"go.ytsaurus.tech/library/go/ptr"


File is not goimports-ed with -local go.ytsaurus.tech (goimports)

actually I think we should remove that rule from linter config, because this library is external to this project.

l0kix2 · 2024-02-28T16:34:48Z

ytsaurus_helpers.go

+			if sourceTypeRaw, ok := ytUser.Attrs[sourceTypeAttributeName]; ok {
+				sourceType = SourceType(sourceTypeRaw.(string))
+			}
+			sourceUser, err = NewSourceUser(sourceType, sourceRaw.(map[string]any))


When I've wrote this code initially I didn't succeed to make the attribute configurable and ytsaurus code knows too much about azure fields, but it is not its area of responsibility (only knowing how to communicate with ytsaurus is). Now since we can configure attribute we can fix that flaw.

I suggest we store map[string]any in YtsaurusUser.SourceData and let app code delegate parsing it to the source (azure/ldap) if needed (I'm actually not sure if we need to parse map to AzureUser/LdapUser at all, because as far as i remember we only serialize one way AzureUser > YtsaurusUser).
I.e. app.buildYtsaurusUser should either call Source method or maybe simpler AzureUser/LdapUser/SourceUser should have a method like AsMap() map[string]any and its result being written in YtsaurusUser.SourceData field.

That way we ensure our components are loosely coupled.

l0kix2 · 2024-02-28T16:35:05Z

ytsaurus_helpers.go

+const (
+	bannedSinceAttributeName = "banned_since"
+	bannedAttributeName      = "banned"
+	sourceTypeAttributeName  = "source_type"


We don't need to store that in ytsaurus, we have it in config.

No, we don't. In config we have only SourceAttributeName (for "azure" or "source" values).

my bad, but still think since source is configured in the app we can delegate handling source-fields to that source without storing it in attributes.

l0kix2 · 2024-02-28T16:53:56Z

app_test.go

+			FirstName:     aliceAzure.FirstName,
+			LastName:      aliceAzure.LastName,
+			DisplayName:   aliceAzure.DisplayName,
+		},


Lets simplify diff for tests by introducing some helper structs like testYtsaurusAzureUser with same fields as current YtsaurusUser and call converter function where needed in test code.

l0kix2

Haven't checked tests changes and diff algorithm changes yet, only main code structure.

l0kix2 · 2024-03-06T09:39:28Z

ytsaurus_models.go

+	SourceGroup SourceGroup
+	// Members is a set of strings, representing users' ObjectID.
+	Members StringSet
+}


These interfaces/structs are belong to app files, not to ytsaurus files. Particullary to diff.go since they used only there.

l0kix2 · 2024-03-06T09:58:40Z

diff.go

+	if ytUser.IsManuallyManaged(a.source.GetSourceType()) {
+		return nil, errors.New("user is manually managed and can't be converted to source user")
+	}
+	sourceType := SourceType(*ytUser.SourceType)


As I said earlier I don't see the need to duplicate source type (which is already configured in config file) in each user in ytsaurus.
I suggest we simple add NewUser method in Source interface, implement it in azure source (calling NewAzureUser) and here we may simply call a.source.NewUser.

That way code would be cleaner because:

we don't need to duplicate and store source type in all users (and we don't need to migrate our data).

we don't need to check source type in the runtime — source method existence would be checked in the compile time

we don't need to support list of possible source types here.

l0kix2 · 2024-03-06T10:42:21Z

ytsaurus_helpers.go

+				}
+			}
+			if sourceRaw, ok := ytUser.Attrs[sourceAttributeName]; ok {
+				// For backward compatibility only.


I think we can break backward compatibility here since I am the only app user currently.
Just lets check that sourceAttribute is not empty at the app start.

l0kix2 · 2024-03-06T10:43:12Z

config.go

@@ -73,6 +76,8 @@ type YtsaurusConfig struct {
 	DebugUsernames []string `yaml:"debug_usernames"`
 	// DebugGroupnames is a list of YTsaurus groupnames for which app will print more debug info in logs.
 	DebugGroupnames []string `yaml:"debug_groupnames"`
+
+	SourceAttributeName *string `yaml:"source_attribute_name"`


Please add a comment that it will be the attribute name of user object in ytsaurus.

l0kix2 · 2024-03-06T10:43:45Z

config.go

@@ -73,6 +76,8 @@ type YtsaurusConfig struct {
 	DebugUsernames []string `yaml:"debug_usernames"`
 	// DebugGroupnames is a list of YTsaurus groupnames for which app will print more debug info in logs.
 	DebugGroupnames []string `yaml:"debug_groupnames"`
+
+	SourceAttributeName *string `yaml:"source_attribute_name"`


Let's make it simple string as we discussed + add != "" check in app start (ytsaurus object creation to be specific).

l0kix2 · 2024-03-06T11:05:28Z

ytsaurus_models.go

-func (u YtsaurusUser) IsManuallyManaged() bool {
-	return u.AzureID == ""
+func (u YtsaurusUser) IsManuallyManaged(sourceType SourceType) bool {
+	return u.SourceRaw == nil || u.SourceType == nil || *u.SourceType != string(sourceType)
 }


Basically managed user must have non empty/non nil SourceRaw, let's just check emptiness of this map. And so we don't accidentally initialise this structure wrongly lets add:

NewAutoManagedYtsaurusUser(username, sourceRaw, bannedSince) function, that would return error if function is called with empty sourceRaw.

NewManualManagedYtsaurusUser(username) for creating manually managed users.

l0kix2 · 2024-03-06T11:55:45Z

diff.go

+	for _, user := range ytUsers {
+		if user.IsManuallyManaged(a.source.GetSourceType()) {
+			continue
+		}


It looks paranoid since GetUsers shouldn't and don't return such users. We must (and maybe already have) have test case for such cases instead of runtime check

l0kix2 · 2024-03-06T11:56:50Z

diff.go

+		}
+		sourceUser, err := a.buildSourceUser(&user)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to create azure user from source")


failed to build source user

l0kix2 · 2024-03-06T12:22:50Z

azure_models.go

+	if err != nil {
+		return nil, err
+	}
+	return &azureUser, nil


This marshall/unmarshall trick is cool, but it may give misleading feeling that AzureUser struct somehow related to yson encoding/decoding. But I think json and any other encoder can be here.

I suppose this small lib https://pkg.go.dev/github.com/mitchellh/mapstructure#example-Decode can help to replace yson marshal/unmarshal boilerplate here and in GetRaw() and same code in Groups and in future ldap source code.

But I not insist on changing this code.

l0kix2 · 2024-03-06T12:26:42Z

azure_real_integration_test.go

@@ -14,7 +14,7 @@ import (
 //go:embed config.local.yaml


syntax in this file and in app_integration_test.go is broken after signature changes.

If you use idea/goland you can add integration in custom tags field in go > build tags setting to see such issues more easily.

l0kix2 · 2024-03-06T12:27:13Z

azure_real_integration_test.go

@@ -14,7 +14,7 @@ import (
 //go:embed config.local.yaml
 var _localConfig embed.FS

-// TestPrintAzureUsersIntegration tests nothing, but can be used to debug Azure users retrieved from ms graph api.
+// TestPrintAzureUsersIntegration tests nothing, but can be used to debug Source users retrieved from ms graph api.


revert back to azure i suppose

l0kix2 · 2024-03-06T12:31:13Z

source_models.go

+
+const (
+	AzureSourceType SourceType = "azure"
+)


I suppose after the changes I suggest earlier we won't need this constant.

l0kix2 · 2024-03-06T12:33:32Z

util.go

+	appTimeFormat            = "2006-01-02T15:04:05Z0700"
+	defaultAzureTimeout      = 3 * time.Second
+	defaultAzureSecretEnvVar = "AZURE_CLIENT_SECRET"
+	defaultAppRemoveLimit    = 10


THis should be in azure_real.go

l0kix2 · 2024-03-06T12:38:33Z

ytsaurus.go

+	if err != nil {
+		return false, err
+	}
+	return attrAzureExists || attrSourceExists, nil


Lets use sourceAttributeName here.

l0kix2

Looks like only minor things left, but let's fix diffUsers a bit (i haven't check diffGroups yet, if it has same issues then let's improve both).

l0kix2 · 2024-03-12T17:10:49Z

config.go

 	// UsersFilter is MS Graph $filter value used for user fetching requests.
 	// See https://learn.microsoft.com/en-us/graph/api/user-list?#optional-query-parameters
 	UsersFilter string `yaml:"users_filter"`
 	// GroupsFilter is MS Graph $filter value used for group fetching requests.
 	// See https://learn.microsoft.com/en-us/graph/api/group-list
-	GroupsFilter string `yaml:"groups_filter"`
+	GroupsFilter string        `yaml:"groups_filter"`
+	Timeout      time.Duration `yaml:"timeout"`


Minor, but don't understand why timeout field is moved between filters.

l0kix2 · 2024-03-12T17:11:58Z

ytsaurus.go

@@ -60,6 +63,9 @@ func NewYtsaurus(cfg *YtsaurusConfig, logger appLoggerType, clock clock.PassiveC
 	if cfg.Timeout == 0 {
 		cfg.Timeout = defaultYtsaurusTimeout
 	}
+	if cfg.SourceAttributeName == "" {
+		cfg.SourceAttributeName = "source"


Better make a const.

l0kix2 · 2024-03-12T17:22:51Z

source_models.go

@@ -0,0 +1 @@
+package main


it seems it is an empty file now and can be deleted :)

l0kix2 · 2024-03-12T18:29:22Z

diff.go

 		create: create,
 		update: update,
 		remove: remove,
-	}
+		result: resultUsersMap,
+	}, nil


Since tests are okey, I assume diffUsers works correctly, but it seem overcomplicated a bit.
I see the implementation that way:
0. create maps sourceUsers [id]sourceUser and ytUsers [id]ytsaurusUser from slices

iterate through sourceUsers map, check ytUsers map and append to create list

iterate through ytUsers map, check sourceUsers map:

not found in sourceUsers go to delete list

changed go to update list.

Here we have
1)existedUsersMap which has two fields and one of them (sourceUser) is not used. So i suggest we rename it to ytUsers [id]ytsaurusUser.
2) ytUsersFromSourceMap which has the same keys as sourceUsers, so we can simple drop building ytUsersFromSourceMap and use sourceUsers for checking and call buildYtsaurusUser only for once we plan to create.

Also resultUsersMap is not being updating in case of user delete, though it should i guess (don't we have a test case for that?)

l0kix2

Just couple of comments folllowing up yesterday review.

Let's fix those small issues I mention and merge.

l0kix2 · 2024-03-13T11:12:37Z

diff.go

+		if err != nil {
+			return nil, errors.Wrap(err, "failed to create azure group from source")
+		}
+		existedGroupsWithMembersMap[sourceGroup.GetID()] = existedGroup{group, sourceGroup}


so existedGroup.sourceGroup is not used therefore code can be little simplified here as in diffUsers i suppose

l0kix2 · 2024-03-13T11:19:17Z

main.go

@@ -36,7 +36,7 @@ func run(configFilePath string) error {
 		return errors.Wrapf(err, "failed to load config %s", configFilePath)
 	}

-	logger, err := configureLogger(cfg.Logging)
+	logger, err := configureLogger(&cfg.Logging)


this change must be supported in azure_real_integration_test.go also

l0kix2 · 2024-03-14T10:26:23Z

diff.go

+			return nil, errors.Wrap(err, "failed to create Ytsaurus user from source user")
+		}
+		ytUsersFromSourceMap[user.GetID()] = ytUser
+	}


I think ytUsersFromSourceMap can be safely removed now, I don't see usages anymore

* sha for docker tag * sha for docker tag #2 * sha for docker tag #3 * sha for docker tag #4

savnadya added 12 commits February 22, 2024 13:16

Introduce sync from ldap

b775be6

Update go.mod and go.sum

8401986

Linter fixes

d08d635

Add LdapConfigTest

38e9184

Ignore users from different source

11fc26f

Fix build

8e056a8

Fix test

1c40a38

Add source_type attribute

4d94268

Review fixes

e728afd

Refactoring only, without ldap related changes

6fbdcaf

Add default value for source_attribute_name

5ca528b

Rename source.go -> source_models.go

95714f9

savnadya requested a review from l0kix2 February 28, 2024 14:28

savnadya added 2 commits February 28, 2024 17:29

Remove commented code

1c6952d

Fix build

2dcbefa

l0kix2 requested changes Feb 28, 2024

View reviewed changes

savnadya added 3 commits March 5, 2024 18:40

Review fixes

614ce40

Fix golangci

c5b4fee

Add buildYtsaurusGroup helper

9261dc1

l0kix2 requested changes Mar 6, 2024

View reviewed changes

Review fixes

2690bae

l0kix2 requested changes Mar 12, 2024

View reviewed changes

l0kix2 requested changes Mar 13, 2024

View reviewed changes

savnadya added 2 commits March 14, 2024 12:53

Review fixes

1ba54fa

Fix test

b039559

l0kix2 requested changes Mar 14, 2024

View reviewed changes

Remove unused map

ae0690d

l0kix2 approved these changes Mar 14, 2024

View reviewed changes

l0kix2 merged commit bfab770 into tractoai:main Mar 14, 2024
1 check passed

l0kix2 added a commit that referenced this pull request Mar 25, 2024

sha for docker tag #4

0fa802f

l0kix2 added a commit that referenced this pull request Mar 25, 2024

git commit sha for docker tag (#9)

1a9a3bc

* sha for docker tag * sha for docker tag #2 * sha for docker tag #3 * sha for docker tag #4

l0kix2 added a commit that referenced this pull request Jun 11, 2024

Fix helm version #4

f6d661a

l0kix2 added a commit that referenced this pull request Jun 11, 2024

Fix helm version #4 (#29)

6e9eb1a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring for adding other sources #4

Refactoring for adding other sources #4

savnadya commented Feb 28, 2024

l0kix2 Feb 28, 2024

l0kix2 Mar 6, 2024 •

edited

Loading

l0kix2 Feb 28, 2024

l0kix2 Feb 28, 2024

l0kix2 Feb 28, 2024

l0kix2 Feb 28, 2024

l0kix2 Feb 28, 2024

savnadya Feb 29, 2024

l0kix2 Feb 29, 2024

l0kix2 Feb 28, 2024

l0kix2 left a comment

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024 •

edited

Loading

l0kix2 Mar 6, 2024 •

edited

Loading

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 Mar 6, 2024

l0kix2 left a comment

l0kix2 Mar 12, 2024

l0kix2 Mar 12, 2024

l0kix2 Mar 12, 2024

l0kix2 Mar 12, 2024

l0kix2 left a comment

l0kix2 Mar 13, 2024

l0kix2 Mar 13, 2024

l0kix2 Mar 14, 2024

Refactoring for adding other sources #4

Refactoring for adding other sources #4

Conversation

savnadya commented Feb 28, 2024

Choose a reason for hiding this comment

l0kix2 Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0kix2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0kix2 Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

l0kix2 Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0kix2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0kix2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0kix2 Mar 6, 2024 •

edited

Loading

l0kix2 Mar 6, 2024 •

edited

Loading

l0kix2 Mar 6, 2024 •

edited

Loading